-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Hints per container input #3416
Conversation
This pull request does not have a backport label. Could you fix it @gizas? 🙏
NOTE: |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one would need some unit tests so as to ensure the added functionality.
🌐 Coverage report
|
Great work @gizas ! I tested it with 1 pod running 2 nginx containers: one with port 80 and another with port 81. I added the annotations to have the correct host for each container: co.elastic.hints/package: nginx
co.elastic.hints/data_streams: stubstatus
co.elastic.hints.nginx-2/host: '${kubernetes.pod.ip}:81'
co.elastic.hints.nginx-1/host: '${kubernetes.pod.ip}:80' And there it was (the barchart is for the unique count of My two containers were there. I also tested with just one host, and it worked. |
Great stuff! Can't wait for this one to be released ;-) |
// mappings.container.name = nginx defines the container we want to emmit the new configuration. Annotations for other containers like co.elastic.hints.webapp should be excluded | ||
func TestGenerateHintsMappingWithProcessorsForContainer(t *testing.T) { | ||
logger := getLogger() | ||
// pod := &kubernetes.Pod{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@constanca-m I commented out the pod section! Just kept it for reference with above tests.
Test is successful
Let me know if is ok or you think we can remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok with me
/test |
3 similar comments
/test |
/test |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just left a couple of small comments.
Co-authored-by: Aman Verma <[email protected]>
Co-authored-by: Aman Verma <[email protected]>
SonarQube Quality Gate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Enhancement
What does this PR do?
This PR is introrduces the functionality described in #3161!
In more details users provide the container name in the hint and this functionality is applied only to a specific container of the pod. When a pod has multiple containers, the settings are shared unless you put the container name in the hint.
For eg.
co.elastic.hints.nginx/stream: stderr
is an annotation (aka. hints) that will be applied to a container wtih name nginxWhy is it important?
It introduces a missing functionality that exists in Hints autodiscovry for beats as described in the Multiple Containers Section
Checklist
./changelog/fragments
using the changelog toolHow to test this PR locally
kubectl exec -ti -n kube-system elastic-agent-t95bj -- bash /share/elastic-agent# elastic-agent inspect -v --variables --variables-wait 2s
Related issues
Logs
Output file generated with inspect command
agent.txt